-
Notifications
You must be signed in to change notification settings - Fork 40
getNearestSelectionPosition changes #717
Conversation
… to getNearestSelectionRange. Added tests and required fixes to other methods using this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit short on time now, so I made a short review too.
selection.setRanges( insertion.getSelectionRanges() ); | ||
const newRange = insertion.getSelectionRange(); | ||
|
||
/* istanbul ignore else */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have every case like this commented. We're not testing else because it's a safe check for some unpredictable edge cases. Also, having a log.warning()
would be good there, like in the other cases in this method.
* * `forward` - searching will be performed only forward, | ||
* * `backward` - searching will be performed only backward. | ||
* | ||
* When valid selection range cannot be found, `null` value is returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"null
is returned"
if ( this.schema.check( { name: '$text', inside: step.value.nextPosition } ) ) { | ||
return step.value.nextPosition; | ||
} | ||
for ( let data of getWalkersData( backwardWalker, forwardWalker ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Walker has no data. Walker walks ;). Maybe combineWalkers()
?
Added requested fixes. |
|
||
test( | ||
'should return null if there is no valid range', | ||
'<image></image>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case is a bit strange. Isn't this pattern missing initial selection? And if it does, shouldn't the image be selected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK... Image is not an object in this test suite. This is confusing. Could we name it like... damn, I can't find a name, because this case shouldn't really occur :D. Perhaps we can call it <emptyBlock>
then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case should be changed indeed. Initial selection should be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor thing to correct in the tests.
Added requested changes. |
There's a warning logged by the tests:
See https://travis-ci.org/ckeditor/ckeditor5-engine/builds/183855169 If it's expected that it's logged, it should be stubbed. If not, then it means that either we don't need that istanbul comment or that something works incorrectly. |
Fixes ckeditor/ckeditor5#3907.